Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feature(gatsby): Support TypeScript by default by compiling TS files #23547

Merged
merged 15 commits into from
May 5, 2020

Conversation

blainekasten
Copy link
Contributor

@blainekasten blainekasten commented Apr 28, 2020

Description

🎉We are working on supporting TypeScript as a first class citizen within Gatsby 🎉
We already have the gatsby-plugin-typescript plugin that adds compilation support for .ts(x) files. We've decided that as we add more support for TypeScript, we want to do it via the plugin. So our first step is to make that plugin part of the default gatsby set up.

This PR does the following:

  1. Adds gatsby-plugin-typescript to the loaded plugins with no custom options if not provided by the users gatsby-config.
  2. Defers to the user's gatsby-config if it includes a gatsby-plugin-typescript entry.

Documentation

Docs exist here: https://www.gatsbyjs.org/packages/gatsby-plugin-typescript/
Do we want to add some banner that this is auto-included in gatsby and that customization can be done by adding it to their config?

Related Issues

#18983

@blainekasten blainekasten requested a review from a team as a code owner April 28, 2020 03:05

let plugins = await loadPlugins(config)

plugins = replaceFieldsThatCanVary(plugins)
Copy link
Contributor

@wardpeet wardpeet Apr 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to create a different helper function here? The one that exists now also check paths property and might cause weird test failures when we might move more plugins as a default.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure. Do you have any suggestions? It seems to work right now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's the same setup we have for gatsby-source-page-creators

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe replaceFieldsThatCanVary should also just do more checks before trying to do pluginOptions.path sanitization (apply it only for gatsby-plugin-page-creator)?

But this doesn't seem to be concern of this pull request?

Copy link
Contributor

@wardpeet wardpeet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! I would ship this PR without any of the modifications I'm going to list here as it's more tech depth and might complicate the PR review a bit more.

Maybe in a follow-up PR we could clean up the load-plugins function a bit.

  • Refactor config.plugins check to only happen once. Instead of x amount of times. I would slap a default [] on it.
  • extract the require.resolve and plugin find check into a helper function so it can be re-used.
  • Move default plugins to an array so we can loop through it instead of duplicating these code paths.

@blainekasten
Copy link
Contributor Author

@wardpeet great idea, I created a clubhouse ticket to follow up on that refactor work

DSchau
DSchau previously approved these changes Apr 28, 2020
Copy link
Contributor

@DSchau DSchau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great -- we'll definitely want to document this (gatsby-plugin-typescript documentation should change), and I honestly think this is shoutable with a blog post, as well.

cc @hashimwarren

pieh
pieh previously approved these changes Apr 28, 2020
Copy link
Contributor

@pieh pieh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, this is exciting!

@pieh
Copy link
Contributor

pieh commented Apr 28, 2020

Maybe in a follow-up PR we could clean up the load-plugins function a bit.

Refactor config.plugins check to only happen once. Instead of x amount of times. I would slap a default [] on it.
extract the require.resolve and plugin find check into a helper function so it can be re-used.
Move default plugins to an array so we can loop through it instead of duplicating these code paths.

@wardpeet But is this something that we will actually ever do? We do plugins iterations almost any time we call apiRunnerNode (like we do this for each created node for each plugin implementing onCreateNode), so I think while this is good idea in general - if there are so many plugins that this would cause problems - the few extra loops we run once (when loading config) would be least of our worries?

Is the extra abstraction worth doing here? Current code is pretty straight forward I think and arguably might be easier to reason with than the added utilities which would have their own API etc

@pieh pieh dismissed their stale review April 28, 2020 20:55

Actually, we should likely make some docs adjustments in this PR as well before merging this in

pieh
pieh previously requested changes Apr 28, 2020
Copy link
Contributor

@pieh pieh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think code-wise this is good to go. However we likely need some documentation updates as well?

@LekoArts
Copy link
Contributor

LekoArts commented Apr 30, 2020

Documentation should also include the note that you need to add it to your own gatsby-config.js so that you add the options that the plugin accepts.

A "Using TypeScript" doc then would only prompt you do init a tsconfig and install typescript + @types

@blainekasten blainekasten requested a review from a team as a code owner April 30, 2020 21:30
Copy link

@AishaBlake AishaBlake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking great. Thanks for talking with me about how to document this change, @blainekasten! I've left a few comments. Could you also add the TypeScript doc to the sidebar under Reference Guides? You'll need to add it to doc-links.yaml.

Everything that follows are suggestions and shouldn't block this PR. For the reference guide, I'd call out that people interested in trying TypeScript don't need to switch their whole app over all at once. (I've never actually done this on an app in production. You're a much better person to guide folks on how to move an existing app to TypeScript.) I'd also talk a little bit about why someone might want to use TypeScript with their Gatsby site. This needn't be more than a sentence or two. I would imagine it could do double duty and also touch on why we're supporting TypeScript by default.

packages/gatsby-plugin-typescript/README.md Outdated Show resolved Hide resolved
packages/gatsby-plugin-typescript/README.md Outdated Show resolved Hide resolved
docs/docs/typescript.md Show resolved Hide resolved
@blainekasten blainekasten requested a review from a team as a code owner May 1, 2020 18:25
@blainekasten
Copy link
Contributor Author

@AishaBlake I gave it another pass and included all of your suggestions. Want to take another gander?

@blainekasten blainekasten dismissed stale reviews from pieh and AishaBlake May 1, 2020 18:26

made updates

AishaBlake
AishaBlake previously approved these changes May 1, 2020
Copy link

@AishaBlake AishaBlake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tweaked the wording of one sentence to use "you" as the pronoun per the style guide. This looks good!


## Introductory paragraph

[TypeScript](https://www.typescriptlang.org/) is a JavaScript superset which extends the language to include type definitions allowing codebases to be statically checked for soundness. Gatsby provides an integrated experience out of the box, similar to an IDE. If you are new to TypeScript, adoption can _and should_ be incremental. Since Gatsby natively supports JavaScript and TypeScript, you can change files from .js to .tsx at any point to start adding types and gaining the benefits of a type system.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔥 Yes! This hits all the points I had in mind!

@blainekasten blainekasten merged commit 1933230 into master May 5, 2020
@delete-merged-branch delete-merged-branch bot deleted the typescript-by-default/compile-ts branch May 5, 2020 18:43
If you are new to TypeScript, check out these other resources to learn more:

- [TypeScript Documentation](https://www.typescriptlang.org/docs/handbook/basic-types.html)
- [TypeScript Playground (Try it out!)](https://www.typescriptlang.org/play/index.html)
Copy link
Contributor

@orta orta May 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gatsby's API is available in the Playground, so this could be more like this link instead

@RIP21
Copy link
Contributor

RIP21 commented May 7, 2020

BTW. Since you make TS as a first-class citizen etc. Please take a look and address this issue.
#16310

Integrating GraphQL Codegen into Gatsby or making an official plugin for it will be an awesome thing too because that will make Gatsby e2e typed automatically.

Please address that if you really want THE BEST dev experience for Typescript projects.

fafnirical added a commit to fafnirical/developing.ninja that referenced this pull request May 24, 2020
gatsby-plugin-typescript is included in Gatsby's default configuration

see gatsbyjs/gatsby#23547
pragmaticpat pushed a commit to pragmaticpat/gatsby that referenced this pull request Apr 28, 2022
…atsbyjs#23547)

* feature(gatsby): Support TypeScript by default by compiling TS files

* add a test to ensure we arent adding an extra gatsby-plugin-typescript

* Make a TypeScript file in gatsby-starter

* Add TypeScript example to blog starter

* Update Using Typescript example

* update gatsby-plugin-typescript README.md

* Add Basic TypeScript example

* Update packages/gatsby-plugin-typescript/README.md

Co-authored-by: Aisha Blake <aisha@gatsbyjs.com>

* Update packages/gatsby-plugin-typescript/README.md

Co-authored-by: Aisha Blake <aisha@gatsbyjs.com>

* Add more to the TS docs

* Add docs to sidebar

* Use you as the pronoun

* Fix e2e test

Co-authored-by: Aisha Blake <aisha@gatsbyjs.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants